Skip to content

Use app.restApiRoot - #10

Merged
bajtos merged 3 commits into
masterfrom
feature/use-restApiRoot
Jan 14, 2014
Merged

Use app.restApiRoot#10
bajtos merged 3 commits into
masterfrom
feature/use-restApiRoot

Conversation

@bajtos

@bajtos bajtos commented Jan 8, 2014

Copy link
Copy Markdown
Member

This pull request contains commits from #9 relevant to the new config option restApiRoot.

/to: @ritch please review.

Miroslav Bajtoš added 2 commits January 7, 2014 16:13
Use `app.docs()` to expose Swagger specs. This way we don't have to
depend on loopback's dependency strong-remoting.
@slnode

slnode commented Jan 8, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-explorer/39/

Comment thread index.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to deprecate app.docs in the core application in favor of using swagger directly here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it better to access swagger directly? Things I don't like about that approach: The consumer has to know intimate details about how loopback is composed to be able to find strong-remoting and the swagger extension (requireLoopbackDependency('strong-remoting/ext/swagger') is a serious code smell). It also has to know that loopback is using strong-remoting and the swagger builder requires a list of remotable methods (as opposed to e.g. list of shared classes). If we later decided to extend swagger descriptors with e.g. model definitions and add another parameter to swagger() function, then we will break compatibility of new strong-remoting/loopback with the old loopback-explorer. It wouldn't happen with app.docs() approach, because it's abstracting away all these details.

If we want to deprecate app.docs then I would prefer to either deprecate strong-remoting/ext/swagger too and move the code to this project, or expose strong-remoting/ext/swagger in loopback exports. E.g. loopback.remoting.ext.swagger, where loopback.remoting is require('strong-remoting') and the rest is filled by strong-remoting.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should deprecate strong-remoting/ext/swagger and move that code into this project. That sounds like a project on its own, so depending on app.docs for this change is reasonable.

@bajtos

bajtos commented Jan 10, 2014

Copy link
Copy Markdown
Member Author

I have created #11 to track this idea.

Is there anything else to improve? May I merge?

@ritch

ritch commented Jan 10, 2014

Copy link
Copy Markdown
Member

LGTM

@slnode

slnode commented Jan 14, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-explorer/174/

@bajtos

bajtos commented Jan 14, 2014

Copy link
Copy Markdown
Member Author

test please

@slnode

slnode commented Jan 14, 2014

Copy link
Copy Markdown

Test FAILed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-explorer/176/

@bajtos

bajtos commented Jan 14, 2014

Copy link
Copy Markdown
Member Author

The build says it has installed loopback from master (which is at v1.5.0 at the moment) but the installed version is v1.4.2:

Installed: loopback@1.4.2 from origin/master

/cc @rmg

@bajtos

bajtos commented Jan 14, 2014

Copy link
Copy Markdown
Member Author

test please

@slnode

slnode commented Jan 14, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback-explorer/177/

@bajtos

bajtos commented Jan 14, 2014

Copy link
Copy Markdown
Member Author

The build is passing now. A possible explanation is that loopback v1.5.0 was not yet published to our artifactory at the time when the previous builds were running.

bajtos added a commit that referenced this pull request Jan 14, 2014
@bajtos
bajtos merged commit 6615013 into master Jan 14, 2014
@bajtos
bajtos deleted the feature/use-restApiRoot branch January 14, 2014 08:21
@rmg

rmg commented Jan 14, 2014

Copy link
Copy Markdown
Member

@bajtos that seems likely. Another cause would be if package.json listed 1.4.x, which would cause npm to replace the 1.5.0 from Artifactory with a 1.4.x from npmjs.org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants